refactor: Add initial wrapper classes for all SLEs - #7886
Open
mvadari wants to merge 8 commits into
Open
Conversation
mvadari
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new SLEBase<ViewT> template and a set of thin, per-ledger-entry wrapper classes (e.g., AccountRootEntry<ViewT>, OfferEntry<ViewT>) intended as the first step toward replacing direct use of raw STLedgerEntry/SLE across the codebase.
Changes:
- Added
include/xrpl/ledger/helpers/SLEBase.h, providing a view-parameterized wrapper with compile-time gated writable operations. - Added initial wrapper headers for a broad set of ledger entry (SLE) types, each providing a convenience constructor that resolves the entry from a
Keylet/keylet helper against a view. - Established
ReadOnlySLEandWritableSLEaliases as generic wrappers for unknown concrete ledger entry types.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 32 comments.
Show a summary per file
| File | Description |
|---|---|
| include/xrpl/ledger/helpers/SLEBase.h | Introduces the core view-parameterized base wrapper for SLE access/mutation. |
| include/xrpl/ledger/helpers/AccountRootEntry.h | Adds an AccountRoot SLE wrapper with a keylet::account constructor. |
| include/xrpl/ledger/helpers/AMMEntry.h | Adds an AMM SLE wrapper with a keylet::amm constructor. |
| include/xrpl/ledger/helpers/AmendmentsEntry.h | Adds an Amendments SLE wrapper with a keylet::amendments constructor. |
| include/xrpl/ledger/helpers/BridgeEntry.h | Adds a Bridge SLE wrapper with a keylet::bridge constructor. |
| include/xrpl/ledger/helpers/CheckEntry.h | Adds a Check SLE wrapper with a keylet::check constructor. |
| include/xrpl/ledger/helpers/CredentialEntry.h | Adds a Credential SLE wrapper with a keylet::credential constructor. |
| include/xrpl/ledger/helpers/DelegateEntry.h | Adds a Delegate SLE wrapper with a keylet::delegate constructor. |
| include/xrpl/ledger/helpers/DepositPreauthEntry.h | Adds a DepositPreauth SLE wrapper with a keylet::depositPreauth constructor. |
| include/xrpl/ledger/helpers/DIDEntry.h | Adds a DID SLE wrapper with a keylet::did constructor. |
| include/xrpl/ledger/helpers/DirectoryNodeEntry.h | Adds a DirectoryNode SLE wrapper with a keylet::ownerDir constructor. |
| include/xrpl/ledger/helpers/EscrowEntry.h | Adds an Escrow SLE wrapper with a keylet::escrow constructor. |
| include/xrpl/ledger/helpers/FeeSettingsEntry.h | Adds a FeeSettings SLE wrapper with a keylet::feeSettings constructor. |
| include/xrpl/ledger/helpers/LedgerHashesEntry.h | Adds a LedgerHashes/SkipList SLE wrapper with a keylet::skip constructor. |
| include/xrpl/ledger/helpers/LoanBrokerEntry.h | Adds a LoanBroker SLE wrapper with a keylet::loanBroker constructor. |
| include/xrpl/ledger/helpers/LoanEntry.h | Adds a Loan SLE wrapper with a keylet::loan constructor. |
| include/xrpl/ledger/helpers/MPTokenEntry.h | Adds an MPToken SLE wrapper with a keylet::mptoken constructor. |
| include/xrpl/ledger/helpers/MPTokenIssuanceEntry.h | Adds an MPTokenIssuance SLE wrapper with a keylet::mptokenIssuance constructor. |
| include/xrpl/ledger/helpers/NegativeUNLEntry.h | Adds a NegativeUNL SLE wrapper with a keylet::negativeUNL constructor. |
| include/xrpl/ledger/helpers/NFTokenOfferEntry.h | Adds an NFTokenOffer SLE wrapper with a keylet::nftokenOffer constructor. |
| include/xrpl/ledger/helpers/NFTokenPageEntry.h | Adds an NFTokenPage SLE wrapper with a keylet::nftokenPage constructor. |
| include/xrpl/ledger/helpers/OfferEntry.h | Adds an Offer SLE wrapper with a keylet::offer constructor. |
| include/xrpl/ledger/helpers/OracleEntry.h | Adds an Oracle SLE wrapper with a keylet::oracle constructor. |
| include/xrpl/ledger/helpers/PayChannelEntry.h | Adds a PayChannel SLE wrapper with a keylet::payChannel constructor. |
| include/xrpl/ledger/helpers/PermissionedDomainEntry.h | Adds a PermissionedDomain SLE wrapper with a keylet::permissionedDomain constructor. |
| include/xrpl/ledger/helpers/RippleStateEntry.h | Adds a RippleState SLE wrapper with a keylet::trustLine constructor. |
| include/xrpl/ledger/helpers/SignerListEntry.h | Adds a SignerList SLE wrapper with a keylet::signerList constructor. |
| include/xrpl/ledger/helpers/SponsorshipEntry.h | Adds a Sponsorship SLE wrapper with a keylet::sponsorship constructor. |
| include/xrpl/ledger/helpers/TicketEntry.h | Adds a Ticket SLE wrapper with a keylet::ticket constructor. |
| include/xrpl/ledger/helpers/VaultEntry.h | Adds a Vault SLE wrapper with a keylet::vault constructor. |
| include/xrpl/ledger/helpers/XChainOwnedClaimIDEntry.h | Adds an XChainOwnedClaimID SLE wrapper with a keylet::xChainClaimID constructor. |
| include/xrpl/ledger/helpers/XChainOwnedCreateAccountClaimIDEntry.h | Adds an XChainOwnedCreateAccountClaimID SLE wrapper with a keylet::xChainCreateAccountClaimID constructor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| { | ||
| // Views are never const objects; the read-only wrapper only holds a const | ||
| // reference because it does not itself modify the view. | ||
| if (auto const applyView = dynamic_cast<ApplyView*>(const_cast<ReadView*>(&view))) |
Contributor
There was a problem hiding this comment.
const_cast mutates const ReadView through peek(). Require explicit non-const ref for peek-based resolution:
inline SLE::const_pointer
resolveEntry(ReadView const& view, Keylet const& key)
{
return view.read(key);
}
| requires kIsWritable | ||
| { | ||
| XRPL_ASSERT(canModify(), "xrpl::SLEBase::erase : can modify"); | ||
| view_.erase(sle_); |
Contributor
There was a problem hiding this comment.
erase() doesn't reset sle_ to nullptr; subsequent calls operate on stale entry. Reset to null:
void
erase()
requires kIsWritable
{
XRPL_ASSERT(canModify(), "xrpl::SLEBase::erase : can modify");
view_.erase(sle_);
sle_ = nullptr;
}
| else | ||
| { | ||
| XRPL_ASSERT(exists(), "xrpl::SLEBase::keylet : exists"); | ||
| return Keylet(sle_->getType(), sle_->key()); |
Contributor
There was a problem hiding this comment.
keylet() relies on debug-only assertions; crashes in release builds. Use runtime check when !exists():
[[nodiscard]] Keylet
keylet() const
{
if constexpr (kIsWritable)
{
return key_;
}
else
{
if (!exists())
throw std::logic_error("keylet(): entry does not exist");
return Keylet(sle_->getType(), sle_->key());
}
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Title pretty much says it all - this PR adds some wrapper classes for all ledger entry types, intended to replace the use of raw SLEs.
Context of Change
A production-grade Step 1 version of #7791
API Impact
N/A